Conversation
code-crusher
commented
Feb 3, 2026
- fix multi-window auth sync
- update changelog
There was a problem hiding this comment.
🧪 PR Review is completed: The implementation of cross-window secret synchronization using secrets.onDidChange is the correct approach. However, there are opportunities to improve robustness by adding error handling and removing redundant focus-based refreshes.
Skipped files
CHANGELOG.md: Skipped file pattern
| this.secretsChangeDisposable = this.originalContext.secrets.onDidChange(async () => { | ||
| await this.refreshSecrets() | ||
| if (this.onSecretChangeCallback) { | ||
| await this.onSecretChangeCallback() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🟡 Code Quality
Issue: The event listener for secret changes lacks error handling. If refreshSecrets or the callback fails, it could lead to unhandled promise rejections.
Fix: Wrap the execution in a try-catch block to safely handle any errors during the update process.
Impact: Prevents silent failures and unhandled rejections
| this.secretsChangeDisposable = this.originalContext.secrets.onDidChange(async () => { | |
| await this.refreshSecrets() | |
| if (this.onSecretChangeCallback) { | |
| await this.onSecretChangeCallback() | |
| } | |
| }) | |
| this.secretsChangeDisposable = this.originalContext.secrets.onDidChange(async () => { | |
| try { | |
| await this.refreshSecrets() | |
| if (this.onSecretChangeCallback) { | |
| await this.onSecretChangeCallback() | |
| } | |
| } catch (error) { | |
| console.error("Failed to handle secret change:", error) | |
| } | |
| }) |
| // Multi-window synchronization: refresh secrets when window gains focus | ||
| const windowStateDisposable = vscode.window.onDidChangeWindowState(async (e) => { | ||
| if (e.focused && this.contextProxy.isInitialized) { | ||
| await this.contextProxy.refreshSecrets() | ||
| await this.postStateToWebview() | ||
| } | ||
| }) | ||
| this.disposables.push(windowStateDisposable) |
There was a problem hiding this comment.
🟡 Performance
Issue: The onDidChangeWindowState listener triggers a secret refresh every time the window gains focus. Since secrets.onDidChange (handled in ContextProxy) already provides reliable cross-window synchronization, this focus handler creates unnecessary overhead.
Fix: Remove the redundant focus listener and rely on the event-driven synchronization.
Impact: Reduces unnecessary IPC calls and secret storage reads
| // Multi-window synchronization: refresh secrets when window gains focus | |
| const windowStateDisposable = vscode.window.onDidChangeWindowState(async (e) => { | |
| if (e.focused && this.contextProxy.isInitialized) { | |
| await this.contextProxy.refreshSecrets() | |
| await this.postStateToWebview() | |
| } | |
| }) | |
| this.disposables.push(windowStateDisposable) |